-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Enhancement] Support push down aggregate functions in mv rewrite #49979
base: main
Are you sure you want to change the base?
[Enhancement] Support push down aggregate functions in mv rewrite #49979
Conversation
...n/java/com/starrocks/sql/optimizer/rule/transformation/materialization/EquationRewriter.java
Show resolved
Hide resolved
...tarrocks/sql/optimizer/rule/transformation/materialization/AggregateFunctionRollupUtils.java
Show resolved
Hide resolved
e8ef9ad
to
3a360a4
Compare
@mergify rebase |
✅ Branch has been successfully rebased |
248da0f
to
39d686a
Compare
940cb58
to
c004596
Compare
Signed-off-by: shuming.li <[email protected]>
c004596
to
b6f7885
Compare
Quality Gate passedIssues Measures |
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 104 / 112 (92.86%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
// if fn is a scalar function, it can be pushed down to mv union rewrite. | ||
public static final Map<String, String> MV_REWRITE_PUSH_DOWN_FUNCTION_MAP = ImmutableMap.<String, String>builder() | ||
// Functions and rollup functions are the same. | ||
.put(FunctionSet.SUM, FunctionSet.SUM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not support COUNT?
return rewriter.rewrite(arg0); | ||
} else { | ||
// if there are many column refs in arg0, the agg column must be the same. | ||
if (arg0Call.getFnName().equalsIgnoreCase(FunctionSet.IF)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about other functions: ifnull, nullif, coalesce?
} else { | ||
// if there are many column refs in arg0, the agg column must be the same. | ||
if (arg0Call.getFnName().equalsIgnoreCase(FunctionSet.IF)) { | ||
if (arg0Call.getChildren().size() != 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IF functions's children num must be 3, no need check!
return null; | ||
} | ||
// constant operator | ||
if (colRefs.size() == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about const null operator?
sum(null) seems should be rewritten into 0;
max(null) seems should be rewritten into null;
Why I'm doing:
For simple aggregate functions(eg: min/max/sum), we can push down the agg function even if it's not exactly matched:
eg:
What I'm doing:
if(cond, val1, val2)
andcase when
rewrite test cases.Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: